Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate existing theme supports to configure the editor to theme.json configs #24761

Merged
merged 5 commits into from
Aug 28, 2020

Conversation

youknowriad
Copy link
Contributor

Related #20588

The idea of this PR is that there's a lot of existing theme supports like disable-custom-colors... that can be used to control Gutenberg and we want to consolidate all of these in a unique "features" key that can be provided by theme.json or block-editor-settings filter.

This PR explores how we can do so while still retaining backward compatibility for existing theme supports the block editor settings. It explores doing so for the disableCustomColors setting. It is now transformed to a feature with the following path: colors.allowCustom.

Also to do so, it adds the global styles settings to the new screens (navigation, widgets, edit-site).

Testing instructions

  • Check that you can enable/disable custom colors support using the existing theme support flag disable-custom-colors
  • Check that you can enable/disable custom colors support using the theme.json colors.allowCustom path.

@youknowriad youknowriad added [Type] Experimental Experimental feature or API. Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json labels Aug 24, 2020
@youknowriad youknowriad self-assigned this Aug 24, 2020
@youknowriad youknowriad requested a review from oandregal August 24, 2020 11:28
@youknowriad youknowriad force-pushed the update/start-migrate-theme-supports branch from 5d19198 to 442b296 Compare August 24, 2020 11:29
@@ -127,6 +127,9 @@
"features": {
"typography": {
"dropCap": false
},
"colors": {
"allowCustom": true
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm assuming this file is where we define if the feature is opt-in or opt-out. (Default value provided by Core).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also by reading this, I wonder if it makes more sense to avoid the separation between the top-level "presets" and "features" and just combine them because these are used in conjunction in general.

Components will do things like:

const colorsPreset = useEditorFeature( 'colors.preset' );
const allowCustomColors = useEditorFeature( 'colors.allowCustom' );

// Render palette with these options.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively we can provide a separate hook useEditorPreset( 'colors' ) but it's a bit weird.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a bit weird to me to see the prefix "allow" given cases like "dropCap" when it is implied. I think just custom would be fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed the prefix.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated #20588 by adding a task to look into either creating a usePreset('colors') hook or integrate into the existing useEditorFeature('color.preset').

// Deprecated theme supports
if ( null !== get_theme_support( 'disable-custom-colors' ) ) {
$config['global']['features']['colors']['allowCustom'] = ! get_theme_support( 'disable-custom-colors' );
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is where we added the backward compatibility support for the deprecated theme support flags.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the global features config is empty the previous condition will return:

	if (
		empty( $config['global']['features'] ) ||
		! is_array( $config['global']['features'] )
	) {
		return array();
	}

And in that case, we will not pass the flag value. Should we handle this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be fixed.

settings.disableCustomColors === undefined
? undefined
: ! settings.disableCustomColors,
};
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is where we say which existing block setting maps to which feature path (backward compatibility for block editor settings)

const blockSupportValue = getBlockSupport( blockName, path );
if ( blockSupportValue !== undefined ) {
return blockSupportValue;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this hook should concern itself with getBlockSupport. getBlockSupport is only used to say that a block support a given feature and should be used directly in the "hooks" and not in useEditorFeature which responsibility is more related to block-editor settings and theme.json.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has introduced a regression by which the dropCap control is no longer shown for paragraph #24930

This is also related to this conversation. I think we should merge the fix above so it makes it to this week's release, and then we can figure out how to settle the info coming from block.json, core's theme.json, and theme's theme.json.

@github-actions
Copy link

github-actions bot commented Aug 24, 2020

Size Change: +72 B (0%)

Total Size: 1.17 MB

Filename Size Change
build/block-editor/index.js 126 kB +71 B (0%)
build/format-library/index.js 7.71 kB +1 B
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 kB 0 B
build/annotations/index.js 3.67 kB 0 B
build/api-fetch/index.js 3.44 kB 0 B
build/autop/index.js 2.82 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 7.99 kB 0 B
build/block-directory/style-rtl.css 953 B 0 B
build/block-directory/style.css 952 B 0 B
build/block-editor/style-rtl.css 10.8 kB 0 B
build/block-editor/style.css 10.8 kB 0 B
build/block-library/editor-rtl.css 8.52 kB 0 B
build/block-library/editor.css 8.52 kB 0 B
build/block-library/index.js 136 kB 0 B
build/block-library/style-rtl.css 7.45 kB 0 B
build/block-library/style.css 7.46 kB 0 B
build/block-library/theme-rtl.css 729 B 0 B
build/block-library/theme.css 730 B 0 B
build/block-serialization-default-parser/index.js 1.88 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 47.7 kB 0 B
build/components/index.js 200 kB 0 B
build/components/style-rtl.css 15.7 kB 0 B
build/components/style.css 15.7 kB 0 B
build/compose/index.js 9.67 kB 0 B
build/core-data/index.js 12.3 kB 0 B
build/data-controls/index.js 1.29 kB 0 B
build/data/index.js 8.55 kB 0 B
build/date/index.js 5.38 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 568 B 0 B
build/dom/index.js 4.48 kB 0 B
build/edit-navigation/index.js 11.7 kB 0 B
build/edit-navigation/style-rtl.css 1.16 kB 0 B
build/edit-navigation/style.css 1.16 kB 0 B
build/edit-post/index.js 304 kB 0 B
build/edit-post/style-rtl.css 5.61 kB 0 B
build/edit-post/style.css 5.61 kB 0 B
build/edit-site/index.js 17 kB 0 B
build/edit-site/style-rtl.css 3.06 kB 0 B
build/edit-site/style.css 3.06 kB 0 B
build/edit-widgets/index.js 11.9 kB 0 B
build/edit-widgets/style-rtl.css 2.45 kB 0 B
build/edit-widgets/style.css 2.45 kB 0 B
build/editor/editor-styles-rtl.css 492 B 0 B
build/editor/editor-styles.css 493 B 0 B
build/editor/index.js 45.3 kB 0 B
build/editor/style-rtl.css 3.81 kB 0 B
build/editor/style.css 3.81 kB 0 B
build/element/index.js 4.65 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/style-rtl.css 547 B 0 B
build/format-library/style.css 548 B 0 B
build/hooks/index.js 2.13 kB 0 B
build/html-entities/index.js 621 B 0 B
build/i18n/index.js 3.57 kB 0 B
build/is-shallow-equal/index.js 710 B 0 B
build/keyboard-shortcuts/index.js 2.52 kB 0 B
build/keycodes/index.js 1.94 kB 0 B
build/list-reusable-blocks/index.js 3.12 kB 0 B
build/list-reusable-blocks/style-rtl.css 476 B 0 B
build/list-reusable-blocks/style.css 476 B 0 B
build/media-utils/index.js 5.32 kB 0 B
build/notices/index.js 1.79 kB 0 B
build/nux/index.js 3.4 kB 0 B
build/nux/style-rtl.css 671 B 0 B
build/nux/style.css 668 B 0 B
build/plugins/index.js 2.56 kB 0 B
build/primitives/index.js 1.41 kB 0 B
build/priority-queue/index.js 789 B 0 B
build/redux-routine/index.js 2.85 kB 0 B
build/rich-text/index.js 13.9 kB 0 B
build/server-side-render/index.js 2.77 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.27 kB 0 B
build/url/index.js 4.06 kB 0 B
build/viewport/index.js 1.85 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

@mtias mtias added the Customization Issues related to Phase 2: Customization efforts label Aug 26, 2020
Copy link
Member

@jorgefilipecosta jorgefilipecosta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @youknowriad, I'm not being able to test this PR using theme.json.

In used the global styles test theme. I changed to the following theme.json:

{
	"global": {
		"styles": {
			"color": {
				"background": "var(--wp--preset--color--tertiary)",
				"link": "hotpink"
			}
		},
		"features": {
			"typography": {
				"dropCap": false
			},
			"colors": {
				"allowCustom": false
			}
		}
	},
	"core/paragraph": {
		"styles": {
			"color": {
				"text": "var(--wp--preset--color--quaternary)"
			},
			"typography": {
				"fontSize": "calc(1px * var(--wp--preset--font-size--normal))",
				"lineHeight": 1.4
			}
		}
	}
}

And the colors continue to be enabled. I'm doing something wrong?

@youknowriad
Copy link
Contributor Author

@jorgefilipecosta Good testing. It was due to two subtle issues that should be fixed with the last commit.

@youknowriad youknowriad force-pushed the update/start-migrate-theme-supports branch from ce8f026 to cb501c1 Compare August 28, 2020 12:01
@youknowriad
Copy link
Contributor Author

If no blockers, I'm thinking of merging this in order to do the same for the remaining flags.

@youknowriad youknowriad merged commit 6b1999e into master Aug 28, 2020
@youknowriad youknowriad deleted the update/start-migrate-theme-supports branch August 28, 2020 14:32
@github-actions github-actions bot added this to the Gutenberg 8.9 milestone Aug 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Customization Issues related to Phase 2: Customization efforts Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json [Type] Experimental Experimental feature or API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants